Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add env vars config for argo-server and workflow-controller #6767

Merged
merged 1 commit into from
Sep 22, 2021
Merged

feat: Add env vars config for argo-server and workflow-controller #6767

merged 1 commit into from
Sep 22, 2021

Conversation

prichrd
Copy link

@prichrd prichrd commented Sep 20, 2021

This is a proposal for #5914. It uses https://github.com/spf13/viper to parse environment variables from the flag names.

I tested this like this:
image
image

@prichrd
Copy link
Author

prichrd commented Sep 20, 2021

Please let me know if this needs more tests.

@alexec
Copy link
Contributor

alexec commented Sep 20, 2021

Looks good, so simple. Can we add some docs about this?

@codecov
Copy link

codecov bot commented Sep 20, 2021

Codecov Report

Merging #6767 (bea64cc) into master (7684ef4) will increase coverage by 0.05%.
The diff coverage is 81.57%.

❗ Current head bea64cc differs from pull request most recent head 38bf4aa. Consider uploading reports for the commit 38bf4aa to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6767      +/-   ##
==========================================
+ Coverage   48.64%   48.70%   +0.05%     
==========================================
  Files         264      265       +1     
  Lines       19076    19108      +32     
==========================================
+ Hits         9279     9306      +27     
- Misses       8752     8757       +5     
  Partials     1045     1045              
Impacted Files Coverage Δ
cmd/argo/commands/server.go 30.14% <50.00%> (-0.63%) ⬇️
workflow/controller/dag.go 76.38% <77.77%> (+0.99%) ⬆️
util/expand/expand.go 100.00% <100.00%> (ø)
workflow/controller/workflowpod.go 74.18% <100.00%> (+0.10%) ⬆️
workflow/validate/validate.go 76.70% <100.00%> (+0.73%) ⬆️
server/workflow/workflow_server.go 44.41% <0.00%> (-2.40%) ⬇️
workflow/controller/operator.go 71.06% <0.00%> (-0.15%) ⬇️
cmd/argo/commands/get.go 58.89% <0.00%> (+0.58%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7684ef4...38bf4aa. Read the comment docs.

@prichrd
Copy link
Author

prichrd commented Sep 20, 2021

@alexec I'll update the following doc this evening:

What doc should I update for the workflow-controller?

@alexec
Copy link
Contributor

alexec commented Sep 20, 2021

I think the right places is probably:

https://github.com/argoproj/argo-workflows/blob/master/docs/environment-variables.md

@alexec
Copy link
Contributor

alexec commented Sep 21, 2021

I was wrong about where to add those flags. It fails make codegen. It also means that we need the keep that list up-to-date - which we will fail to do.

Instead, can I ask you to just add a bit like this:


Parameters can be specified as environment variables:

  workflow-controller --managed-namespace=argo

Can be expressed as:

  env MANAGED_NAMESPACE=argo workflow-controller

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please dismiss review when done.

@prichrd
Copy link
Author

prichrd commented Sep 21, 2021

That makes total sense. I will update the docs/environment-variables.md doc to reflect that suggestion.

@prichrd prichrd requested a review from alexec September 22, 2021 00:03
@alexec
Copy link
Contributor

alexec commented Sep 22, 2021

Is it possible to have ARGO_ as prefix?

@prichrd
Copy link
Author

prichrd commented Sep 22, 2021

It shouldn't be an issue, it is fairly simple to do with Viper's SetEnvPrefix, I'll fix that.

Signed-off-by: Philippe Richard <philippe.richard93@gmail.com>
@prichrd
Copy link
Author

prichrd commented Sep 22, 2021

image
@alexec, done!

@alexec
Copy link
Contributor

alexec commented Sep 22, 2021

🚀

@alexec alexec enabled auto-merge (squash) September 22, 2021 21:47
@alexec alexec merged commit 96c5626 into argoproj:master Sep 22, 2021
@sarabala1979 sarabala1979 mentioned this pull request Sep 28, 2021
36 tasks
kriti-sc pushed a commit to kriti-sc/argo-workflows that referenced this pull request Oct 24, 2021
…goproj#6767)

Signed-off-by: Philippe Richard <philippe.richard93@gmail.com>
Signed-off-by: kriti-sc <kathuriakriti1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants